feat(01KW5WPD): computeSkillEffectiveness resolves retired skills correctly (re-rooted path + injected IO seam) so the retro analyst stops proposing to retire already-deleted or moved skills (#423, #403)#454
Open
jackmcintyre wants to merge 1 commit into
Conversation
…rectly (re-rooted path + injected IO seam) so the retro analyst stops proposing to retire already-deleted or moved skills (#423, #403) Closes #423. Closes #403 (duplicate of #423). ## What The retro analyst was proposing to "retire" skills that had already been deleted (`flow:author`, `flow:judge`, `flow:status`, `flow:board`, `flow:accept-proposal`) because `computeSkillEffectiveness` had no way to filter out retired commands — it tallied every `skill.invoke` event in the 50-story window regardless of whether the skill still exists on disk. ## How - **Injected IO seam**: Added `existsImpl?: (absolutePath: string) => boolean` and `pluginRoot?: string` to `ComputeSkillEffectivenessOptions`. When `existsImpl` is absent the helper is fully backward-compatible (no filtering). Tests inject a fake that never touches the real filesystem (AC2). - **Re-rooted path resolution**: Added `extractSkillCommand` (regex `/[/\\]skills[/\\]([^/\\]+)[/\\]SKILL\.md$/i`) to pull the command name from an absolute, invoke-time-captured `skill_path`. Added `checkInstalled` that probes two candidate dirs: (1) `{targetRepoRoot}/plugins/flow/skills/{cmd}/SKILL.md` (dev/monorepo) and (2) `{pluginRoot}/skills/{cmd}/SKILL.md` (installed-plugin). A live skill whose path moved is NOT misclassified as retired (AC1, AC3). - **Ambiguous-path default**: A `skill_path` matching neither `/skills/{cmd}/SKILL.md` pattern (other-plugin, relative path) defaults to `installed = true` — it is never wrongly dropped (AC4). - **`gatherRetroInputs` wired up**: Now passes `existsImpl: existsSync` and `pluginRoot: getPluginRoot()` (wrapped in try/catch for edge cases), activating the check in production. - **`retro-skill-effectiveness.test.ts` updated**: Existing tests were using retired skills (`flow:author`, `flow:judge`); replaced with live ones (`flow:run`) so they continue to pass after `existsImpl` is active. - **dist rebuilt**: `pnpm build` re-run; `dist/index.js` and `dist/cli.js` committed in the same change (CI fails on drift).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
computeSkillEffectiveness resolves retired skills correctly (re-rooted path + injected IO seam) so the retro analyst stops proposing to retire already-deleted or moved skills (#423, #403)
This change delivers the following acceptance criteria:
Why
As a retro-analyst, I want the skill-effectiveness signal to reflect only skills still installed on disk, resolved correctly against the repo, so that I stop drafting skill-retire proposals for commands that were already removed (flow:author, flow:judge, flow:status, flow:board, crew:board, flow:accept-proposal) WITHOUT wrongly hiding a live skill whose path moved.
Closes #423. Closes #403 (duplicate of #423).
How it works
existsImpl?: (absolutePath: string) => booleanandpluginRoot?: stringtoComputeSkillEffectivenessOptions. WhenexistsImplis absent the helper is fully backward-compatible (no filtering). Tests inject a fake that never touches the real filesystem (AC2).extractSkillCommand(regex/[/\\]skills[/\\]([^/\\]+)[/\\]SKILL\.md$/i) to pull the command name from an absolute, invoke-time-capturedskill_path. AddedcheckInstalledthat probes two candidate dirs: (1){targetRepoRoot}/plugins/flow/skills/{cmd}/SKILL.md(dev/monorepo) and (2){pluginRoot}/skills/{cmd}/SKILL.md(installed-plugin). A live skill whose path moved is NOT misclassified as retired (AC1, AC3).skill_pathmatching neither/skills/{cmd}/SKILL.mdpattern defaults toinstalled = true— never wrongly dropped (AC4).gatherRetroInputswired up: Now passesexistsImpl: existsSyncandpluginRoot: getPluginRoot()(wrapped in try/catch for edge cases), activating the check in production.retro-skill-effectiveness.test.tsupdated: Existing tests were using retired skills (flow:author,flow:judge); replaced with live ones (flow:run) so they continue to pass afterexistsImplis active.pnpm buildre-run;dist/index.jsanddist/cli.jscommitted in the same change (CI fails on drift).How to check it yourself
Step 1 — Run the targeted test file. From the repo root: cd plugins/flow/mcp-server && pnpm exec vitest run src/tools/tests/compute-skill-effectiveness.test.ts. Expect all tests to pass, including the new describe block "computeSkillEffectiveness — retired-skill installed-check (native:01KW5WPDPJY5DK6JV810307E0J)" with 4 tests covering AC1-AC4.
Step 2 — Run the full test suite. From plugins/flow/mcp-server: pnpm exec vitest run. Expect all approximately 3900 tests to pass with no regressions.
Step 3 — Verify retro-skill-effectiveness tests. From plugins/flow/mcp-server: pnpm exec vitest run src/tools/tests/retro-skill-effectiveness.test.ts. Expect both tests to pass (they now use flow:run instead of the retired flow:author and flow:judge commands).
Step 4 — Verify the production fix. Reinstall the plugin from this branch, open a project with retro telemetry containing invocations of removed commands (e.g. flow:author), then run /flow:retro. Confirm the retro output no longer proposes retiring already-deleted skills like flow:author or flow:judge.
Risk and blast radius
Risk tier: medium
The change is scoped to compute-skill-effectiveness.ts, its test file, gather-retro-inputs.ts, and retro-skill-effectiveness.test.ts. The seam is opt-in (existsImpl absent = no filtering), so all existing callers that do not pass existsImpl are fully backward-compatible. Review the diff to judge its actual blast radius — including any effect on shared state, data schemas, or authentication paths — before approving.
What is explicitly not covered: reviewer verification of this summary's accuracy is handled by a separate companion story.
Evidence
The pre-pull-request build-and-test gate passed before this pull request was opened. No pull request can be opened by the automated flow unless both
pnpm buildandpnpm testexit 0 in the developer's working directory.Per-criterion covering checks:
plugins/flow/mcp-server/src/tools/__tests__/compute-skill-effectiveness.test.ts(automated test)plugins/flow/mcp-server/src/tools/__tests__/compute-skill-effectiveness.test.ts(automated test)plugins/flow/mcp-server/src/tools/__tests__/compute-skill-effectiveness.test.ts(automated test)plugins/flow/mcp-server/src/tools/__tests__/compute-skill-effectiveness.test.ts(automated test)Story ref: native:01KW5WPDPJY5DK6JV810307E0J